fix(server): keep service accounts out of the namespace payload by gustavosbarreto · Pull Request #7129 · shellhub-io/shellhub

gustavosbarreto · 2026-09-17T01:26:08Z

What

The namespace routes now return only human members, and the member object they return
carries no principal type. A service account is a non-human principal that exists for SSH
and is listed by GET /api/service-accounts; its membership is only how authorization
finds it.

Why

Listing a service account as a member broke the contract. It carries the role service,
which the member-role enum does not have, so every namespace response holding one failed
schema validation. The dev log carries both sides of the change on the same route:

WARN  outcome=failed  path=/api/namespaces/{tenant}  Error at "/members/3/role": value "service" is not one of ["administrator","operator","observer","owner"]
DEBU  outcome=passed  path=/api/namespaces/{tenant}  status_code=200

Once the list is people only, the member's type has a single possible value, so it tells
a consumer nothing and comes out of the response.

Closes #7124

Changes

pkg/api/responses/namespace.go: a response type that projects the stored namespace
explicitly, the way CreateAPIKeyFromModel already does, and descends into the member.
An embedded *models.Namespace would do the opposite, leaking every new model field on
its own, and encoding/json skips a nil embedded pointer in silence, so a nil namespace
would serialize as {"members":[]} with a 200. Members is built with make, never
nil, because the schema declares a non-nullable array.
server/api/routes/nsadm.go: the six responses that carry a namespace. The filter
belongs in the transport layer: the store feeds Authorize through FindMember and
filtering there would drop SSH for every service account, and the SSH session calls
GetNamespace in process.
openapi/spec/components/schemas/namespace.yaml: members now says it holds people.
Tests: services/namespace_test.go and the Authorize table in
services/access-policy_test.go were left alone on purpose. They still expect a service
account in the member list, which is what proves the SSH path was not touched.
TestNamespaceMemberSchemaMatchesTheResponse pins the projected member against the
member object the schema publishes, which response validation cannot do: the schema sets
no additionalProperties, so a field added to either side passes.

The predicate is Member.Type, not the role. The role a service account carries is a fixed
stamp written at creation, so an empty type counts as a person. NamespaceGetMembers and
ServiceAccountList already discriminate by type the same way.

Testing

Point a namespace that holds a service account at the running dev stack and read the server
log, not just the payload: the response validator is what caught this, and a regression
shows up there first.

GET /api/namespaces/{tenant}      # members holds people only, and no member carries "type"
GET /api/service-accounts         # still lists every bot

The failure mode worth probing is the opposite direction. A namespace whose only member is
a service account must serialize "members": [], never null, since the schema declares a
non-nullable array. TestNamespaceFromModelNeverMarshalsMembersAsNull pins that, and
dropping the make in the projection is enough to break it.

Follow-ups

shellhub-io/team#247: the instance-admin panel reuses GetNamespace and has no
service-account listing to fall back on, so it needs one. Keeping them in that response
is not an option, since admin@api@namespaces@{tenant}.yaml references this same schema.
fix(openapi): declare the removed device count the namespace carries #7130: devices_removed_count, found while editing this schema and split out of here.

gustavosbarreto · 2026-09-17T01:34:23Z

/review

gustavosbarreto · 2026-09-17T01:44:44Z

/review

gustavosbarreto · 2026-09-17T01:55:17Z

/review

github-actions · 2026-09-17T01:55:38Z

Claude finished @gustavosbarreto's task in 12m 18s —— View job

Code Review Complete

Reviewed 7 files. 3 inline issues posted. Standards: 0 violations, 1 judgement call. Spec: 1. Correctness: 1.

Standards

No rule violations. One judgement call: the projection in pkg/api/responses/namespace.go:15 stops at the top level, so Members []models.Member still carries new model fields to the wire unannounced — the one nested type this PR exists to control, and the drift test doesn't descend into it.

Correctness

GetNamespace (server/api/routes/nsadm.go:118) also backs the instance-admin route via cloud/internal/admin/routes/routes.go:58, and the admin console renders its members table straight off that payload. Service accounts now vanish from the admin panel, which — unlike the tenant API — has no service-account listing to fall back on.

Spec

The issue's requirement is met: all six namespace-returning handlers project through responses.NamespaceFromModel, the store is untouched, and AccessPolicyService.Authorize still resolves service-account memberships, so SSH is intact — the outcome #7124 called for when it warned that "filtering service accounts out of NamespaceResolve would break SSH for them." Option 1 was correctly not taken.

One finding, scope creep: devices_removed_count (openapi/spec/components/schemas/namespace.yaml:127) answers nothing in #7124, and the two ui/apps/console fixture edits in this diff are its fallout rather than the service-account fix's. It already sits in its own commit (6d8aff7).

Additional notes

No findings were dropped by a cap. Five further candidates were discarded in validation, the notable ones being: a suggestion to also gate on authorizer.RoleService, since no reachable path puts a service-account member with an empty Type into these six responses (users.type is NOT NULL DEFAULT 'human' and NamespaceResolve always joins Memberships.User); and the new required entry's reach into the PUT /admin/api/namespaces-update/{tenantID} request body, which follows the three sibling counters already declared required and changes no runtime behaviour, as only ValidateResponse is wired up.

Cross-repo check came back clean otherwise: cloud reuses the core handlers rather than returning models.Namespace itself, and NamespaceUpdate re-reads from the store, so a round-tripped admin edit body with stripped members cannot delete service-account memberships.

To request another review round, comment /review.
• Branch: fix/namespace-members-humans-only

gustavosbarreto · 2026-09-17T02:12:39Z

/review

github-actions · 2026-09-17T02:13:02Z

Claude finished @gustavosbarreto's task in 12m 34s —— View job

Code Review Complete

Reviewed 6 files. 2 inline issues posted. Standards: 0 violations, 2 judgement calls. Spec: 0. Correctness: 0.

Standards

No rule violations. Two judgement calls, both on the tests added to close the last round's drift point: the narrative comment at server/api/openapi/member_schema_test.go:17 duplicates rationale that commit 1f56352's body already carries, and TestNamespaceProjectsEveryWireFieldOfTheModel (pkg/api/responses/namespace_test.go:130) reflects over struct tags without calling NamespaceFromModel, so it pins shape rather than behaviour — dropping Type: m.Type from the projection still passes it, while the adjacent value test catches that.

Spec

Every requirement in #7124 is met. Option 2 was taken as written — "Drop service accounts from the members array when the namespace is serialized, leaving the store untouched" — server/api/store/pg/namespace.go is absent from the diff, NamespaceResolve still loads Memberships.User unfiltered, and AccessPolicyService.Authorize resolves membership off the model rather than the response type, so SSH for service accounts is intact. Option 1 was correctly not taken: namespaceMemberRole.yaml is unchanged. The previous round's devices_removed_count scope creep is gone — the branch is one commit, and namespace.yaml now touches only members.

Additional notes

No findings were dropped by a cap. Both of the last round's remaining points were checked and not re-raised: the admin-route consequence you filed as shellhub-io/team#247, and the member-drift gap, which TestNamespaceMemberSchemaDeclaresEveryWireField now closes — models.Member has exactly seven json-tagged fields and the schema's member item declares exactly those seven, so it passes and discriminates.

Four further candidates were discarded in validation. The two worth naming: the new member type enum is not scope creep, since the issue names "declaring the value the payload really carries" as a legitimate half of the fix and the new schema test fails without that property; and the undeclared devices_removed_count predates this branch — models.Namespace has carried that tag without omitempty since before the fork point, so the handlers were already sending it, and no new test compares namespace-level properties against the YAML. Flagging it here would contradict the split into #7130.

Cross-repo check is clean beyond team#247: cloud/ reuses these handlers rather than serializing models.Namespace itself, the console's access-policy views resolve service-account subjects through /api/service-accounts before falling back to members, and the admin edit round-trip re-resolves from the store and writes only Name, two Settings fields and MaxDevices, so a stripped members array cannot delete a membership.

To request another review round, comment /review.
• Branch: fix/namespace-members-humans-only

gustavosbarreto · 2026-09-17T02:29:17Z

/review

github-actions · 2026-09-17T02:29:49Z

Claude finished @gustavosbarreto's task in 9m 40s —— View job

Code Review Complete

Reviewed 6 files on all three axes: Standards (repo rules, language conventions, tests, code smells), Spec (does it implement the issue?) and Correctness (security and runtime defects). No new issues found. The code looks good as-is.

Notes on this round

Seven candidates were raised and all seven were dropped — two as repeats of threads you already answered (the shape-vs-behaviour test at pkg/api/responses/namespace_test.go:130, now renamed; the member type property read as scope creep, which a prior round had already examined and kept), three below the judgement-call bar, and two discarded in validation:

NamespacesFromModel dereferences *NamespaceFromModel(&namespace) with no nil guard — refuted. &namespace is the address of a loop-local value and is never nil, so the callee's nil branch is structurally unreachable from that call site; adding a guard would be dead code.
namespace.yaml is also the request body of editNamespaceAdmin, so enum: [human] narrows a request contract — the shared $ref is real but predates this branch, and server/api/pkg/openapi/openapi.go wires ValidateResponse only, never ValidateRequest, so the enum has no runtime reach into requests.

Spec coverage came back complete: all six namespace-returning handlers in nsadm.go project, LeaveNamespace returns a *models.UserAuthResponse that carries no members, and no other route in either repo serializes a models.Namespace to a client. The store is untouched, and server/ssh/session/session.go:331 calls GetNamespace in process against *models.Namespace, so SSH still sees service-account memberships.

One limitation worth stating plainly: the review was static. This container has no Docker daemon and no reachable Go toolchain, so go build and go test did not run — the compile and test verdict rests on the QA job, which was still in progress when this review finished.

--- • Branch: fix/namespace-members-humans-only

gustavosbarreto · 2026-09-17T13:12:28Z

/review

gustavosbarreto · 2026-09-17T13:29:34Z

/review

github-actions · 2026-09-17T13:30:04Z

Claude finished @gustavosbarreto's task in 17m 17s —— View job

Code Review Complete

Reviewed 6 files. 3 inline issues posted. Standards: 0 violations, 2 judgement calls. Spec: 0. Correctness: 1.

Standards

No rule violations. Two judgement calls, both in server/api/openapi/namespace_schema_test.go: jsonFieldsOf skips an untagged exported field (line 73) that encoding/json would still write under its Go name, which is the one drift these tests exist to catch; and declaredProperties (48–55) uses the empty string as a mode switch, so a $ref refactor of members.items — the prevailing style in that YAML — fails with a message pointing at the wrong file.

Correctness

pkg/api/responses/namespace.go:52-55 — the console's Access Policies page counts role subjects off this array. ui/apps/console/src/pages/access-policies/index.tsx:230,234 reads ns?.members from useNamespace, which is GET /api/namespaces/{tenant}, not the filtered members endpoint. A policy with subject: {type: role, value: service} now renders a bare service chip instead of service · N, because SubjectCell draws the count only when n > 0. #7124's "the console is not affected either way" covers the members table, not this page. The count is already in scope there via useServiceAccounts().

Spec

Every requirement in #7124 is met. Option 2 was taken as written — the store is untouched (server/api/store/pg/namespace.go is absent from the diff, NamespaceResolve still loads Memberships.User unfiltered), AccessPolicyService.Authorize resolves membership off *models.Namespace rather than the response type, so SSH for service accounts is intact, and Option 1 was correctly not taken (namespaceMemberRole.yaml unchanged). All six namespace-returning handlers project; LeaveNamespace returns a *models.UserAuthResponse, and the remaining namespace handlers return NoContent.

Additional notes

No findings were dropped by a cap. Three candidates were discarded in validation or below the bar:

TestNamespaceSchemaMatchesTheResponse pins the whole top-level property set, beyond the members array the issue is about — dropped. Namespace payload lists service accounts as members with a role the schema forbids #7124 contains no line this contradicts, the commit message states the reason, and it is test-only.
reflect.Type.Fields() may not exist in the declared Go version — settled by CI rather than guessed at. Every QA job is now green, including validate (server), which builds and runs this package.
gopkg.in/yaml.v3 missing from go.mod — refuted. This is a multi-module repo; the import resolves through server/go.mod:33, where it is a direct requirement.

Cross-repo check is clean beyond the console finding above: cloud/ reuses these handlers rather than serializing models.Namespace itself, all ten $refs to namespace.yaml now match the projection field for field, and cloud's NamespaceUpdate ignores Members, so an admin GET→PUT round trip cannot persist the stripped list.

To request another review round, comment /review.
• Branch: fix/namespace-members-humans-only

A service account is a non-human principal that exists for SSH, and its membership is only how authorization finds it. Listing it as a member also broke the contract: it carries the role "service", which the member-role enum does not have, so every namespace response holding one failed schema validation. The filter is a response type rather than a store or service change. The store feeds Authorize through FindMember and would drop SSH for every service account, and the SSH session calls GetNamespace in process. Projecting explicitly, the way CreateAPIKeyFromModel already does, also avoids an embedded pointer, which encoding/json skips in silence when nil. The projection descends into the member, which drops its principal type from the response. Once the list is people only, that field has a single possible value and tells a consumer nothing; a caller that wants the service accounts asks GET /api/service-accounts. Nothing here guards the response against the schema. Response validation cannot, because the schema sets no additionalProperties, so a property the server sends and the schema omits passes. A test per response type is the wrong shape for that job, with 203 operations to cover; #7132 tracks turning the whole document strict at load time instead. The member type is still the discriminator for the filter, not the role: the role is a fixed stamp written at creation, so an empty type counts as a person. NamespaceGetMembers already applies the same rule in SQL for the members list. Fixes: #7124

gustavosbarreto · 2026-09-17T13:55:14Z

/review

github-actions · 2026-09-17T13:55:42Z

Code Review Complete

The automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment /review.

View job

gustavosbarreto requested review from a team as code owners September 17, 2026 01:26

gustavosbarreto force-pushed the fix/namespace-members-humans-only branch 2 times, most recently from d03d5b9 to 6d8aff7 Compare September 17, 2026 01:44

gustavosbarreto requested a review from a team as a code owner September 17, 2026 01:44